Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.59%. Comparing base (4f84d3c) to head (b6423fe).
Report is 1 commits behind head on master.

@@            Coverage Diff             @@
##           master    #6848      +/-   ##
==========================================
- Coverage   98.60%   98.59%   -0.02%     
==========================================
  Files          79       79              
  Lines       14657    14661       +4     
==========================================
+ Hits        14453    14455       +2     
- Misses        204      206       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Restored the translation macros (otherwise potools got a bit confused) and removed the trailing spaces. Please don't add spaces at the end of the line in the future.

@MichaelChirico, would you prefer a test with an ASCII control character separator or a // # nocov here?

By the way, should we be specifying DTPRINT for extraction of translation templates when DTPRINT is defined as Rprintf? I wasn't expecting Rprintf to gettext its arguments, and we do manually translate them in most places.

@MichaelChirico, would you prefer a test with an ASCII control character separator or a // # nocov here?

"neither" :)

I think ultimately it should get a test, but don't see any urgency to do so.

I am fine merging some PRs that have small un-tested sections, esp. if the test is unrelated to the PR at hand.

# nocov OTOH is IMO appropriate only when we think it's not reasonable/impossible to test, or such a test would be useless (e.g. requiring so much mocking as to completely obscure what's actually being tested).

By the way, should we be specifying DTPRINT for extraction of translation templates when DTPRINT is defined as Rprintf? I wasn't expecting Rprintf to gettext its arguments, and we do manually translate them in most places.

I think it's like catf(), used for verbose= messages and thus eligible for translation (as Rprintf() should be).

Oh, I see now. They are extracted thanks to DTPRINT being present in the list, but only marked for translation if the argument is a call to gettext. Messages that are extracted but not marked get reported as untranslated. That's useful.

Right... I guess we could (?) rewrite the macro like

#define DTPRINT(s, ...) Rprintf(_(s), __VA_ARGS__)

But I like the consistency of requiring caller(_(string), ...) in C code. I know some authors prefer bringing that over to the R side to, using a t_() shorthand for gettext() everywhere instead of relying on stop() & friends doing that. But I'm OK w the status quo for now.

The current situation makes perfect sense. I had assumed that the first argument of DTPRINT(...) automatically lands in data.table.pot, but that's not the case. Instead we can either explicitly translate it or mark // #notranslate.